Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add return type to withBoundingRects #1837

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add return type to withBoundingRects #1837

wants to merge 1 commit into from

Conversation

darthmaim
Copy link

@darthmaim darthmaim commented May 25, 2024 •
edited
Loading

🚀 Enhancements

  • Add typescript return type to withBoundingRects

withBoundingRects did not have an explicit return type (as opposed to other higher-order-components like withParentSize).

Typescript generated a complicated return type for components using this HOC (see example below), which was not compatible with react@19 types anymore, because class contexts are no longer supported (not actually used in the HOC).

Now it just returns ComponentClass<Props>, which works with whatever @types/react version the user has installed.

TooltipWithBounds.d.ts Before 
import React from 'react';
import { WithBoundingRectsProps } from '@visx/bounds';
import { TooltipProps } from './Tooltip';
export declare type TooltipWithBoundsProps = TooltipProps & React.HTMLAttributes<HTMLDivElement> & WithBoundingRectsProps & {
    nodeRef?: React.Ref<HTMLDivElement>;
};
declare const _default: {
    new (props: TooltipWithBoundsProps): {
        node: HTMLElement | null | undefined;
        nodeRef: React.RefObject<HTMLElement>;
        componentDidMount(): void;
        getRects(): Readonly<{}>;
        render(): JSX.Element;
        context: any;
        setState<K extends never>(state: {} | Pick<{}, K> | ((prevState: Readonly<{}>, props: Readonly<TooltipWithBoundsProps>) => {} | Pick<{}, K> | null) | null, callback?: (() => void) | undefined): void;
        forceUpdate(callback?: (() => void) | undefined): void;
        readonly props: Readonly<TooltipWithBoundsProps> & Readonly<{
            children?: React.ReactNode;
        }>;
        state: Readonly<{}>;
        refs: {
            [key: string]: React.ReactInstance;
        };
        shouldComponentUpdate?(nextProps: Readonly<TooltipWithBoundsProps>, nextState: Readonly<{}>, nextContext: any): boolean;
        componentWillUnmount?(): void;
        componentDidCatch?(error: Error, errorInfo: React.ErrorInfo): void;
        getSnapshotBeforeUpdate?(prevProps: Readonly<TooltipWithBoundsProps>, prevState: Readonly<{}>): any;
        componentDidUpdate?(prevProps: Readonly<TooltipWithBoundsProps>, prevState: Readonly<{}>, snapshot?: any): void;
        componentWillMount?(): void;
        UNSAFE_componentWillMount?(): void;
        componentWillReceiveProps?(nextProps: Readonly<TooltipWithBoundsProps>, nextContext: any): void;
        UNSAFE_componentWillReceiveProps?(nextProps: Readonly<TooltipWithBoundsProps>, nextContext: any): void;
        componentWillUpdate?(nextProps: Readonly<TooltipWithBoundsProps>, nextState: Readonly<{}>, nextContext: any): void;
        UNSAFE_componentWillUpdate?(nextProps: Readonly<TooltipWithBoundsProps>, nextState: Readonly<{}>, nextContext: any): void;
    };
    displayName: string;
    contextType?: React.Context<any> | undefined;
};
export default _default;
//# sourceMappingURL=TooltipWithBounds.d.ts.map
TooltipWithBounds.d.ts After 
import React from 'react';
import { WithBoundingRectsProps } from '@visx/bounds';
import { TooltipProps } from './Tooltip';
export declare type TooltipWithBoundsProps = TooltipProps & React.HTMLAttributes<HTMLDivElement> & WithBoundingRectsProps & {
    nodeRef?: React.Ref<HTMLDivElement>;
};
declare const _default: React.ComponentClass<TooltipWithBoundsProps, any>;
export default _default;
//# sourceMappingURL=TooltipWithBounds.d.ts.map
Example error before 
Type error: 'TooltipWithBounds' cannot be used as a JSX component.
  Its type '{ new (props: TooltipWithBoundsProps): { node: HTMLElement | null | undefined; nodeRef: RefObject<HTMLElement>; componentDidMount(): void; ... 18 more ...; UNSAFE_componentWillUpdate?(nextProps: Readonly<...>, nextState: Readonly<...>, nextContext: any): void; }; displayName: string; contextType?: Context<...> | und...' is not a valid JSX element type.
    Type '{ new (props: TooltipWithBoundsProps): { node: HTMLElement | null | undefined; nodeRef: RefObject<HTMLElement>; componentDidMount(): void; ... 18 more ...; UNSAFE_componentWillUpdate?(nextProps: Readonly<...>, nextState: Readonly<...>, nextContext: any): void; }; displayName: string; contextType?: Context<...> | und...' is not assignable to type 'new (props: any) => Component<any, any, any>'.
      Construct signature return types '{ node: HTMLElement | null | undefined; nodeRef: RefObject<HTMLElement>; componentDidMount(): void; getRects(): Readonly<{}>; ... 17 more ...; UNSAFE_componentWillUpdate?(nextProps: Readonly<...>, nextState: Readonly<...>, nextContext: any): void; }' and 'Component<any, any, any>' are incompatible.
        The types of 'shouldComponentUpdate' are incompatible between these types.
          Type '((nextProps: Readonly<TooltipWithBoundsProps>, nextState: Readonly<{}>, nextContext: any) => boolean) | undefined' is not assignable to type '((nextProps: Readonly<any>, nextState: Readonly<any>) => boolean) | undefined'.
            Type '(nextProps: Readonly<TooltipWithBoundsProps>, nextState: Readonly<{}>, nextContext: any) => boolean' is not assignable to type '(nextProps: Readonly<any>, nextState: Readonly<any>) => boolean'.
              Target signature provides too few arguments. Expected 3 or more, but got 2.

This change affects the visx-bounds and visx-tooltip packages.

While working on this, I noticed that it is possible to pass all the all the WithBoundingRectsProps to the withBoundingRects(BaseComponent) component, and not just the additional BaseComponent props. withParentSize for example omits all the internal props from the returned component. I can create a separate PR that removes those props like #1783 did for withParentSize. Preview: darthmaim#1

@darthmaim
Copy link
Author

Any feedback by maintainers for this PR?

@darthmaim
Copy link
Author

Just another friendly bump :)

@darthmaim darthmaim mentioned this pull request Dec 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@darthmaim